Open top-level singleton collection by default in hierarchy tree #4201
Conversation
There was a problem hiding this comment.
Thanks for the contribution, @siiriylonen! I think this may require some adjustments, though...
This change will cause every level of the hierarchy tree to be expanded if it is a collection. For large and complex collections, this may result in an overwhelming amount of information to scroll through. Is that really what is intended?
I spoke with @EreMaijala about this, and he thought that the intent might be to only open the top node of the tree if it is a collection and other items are not open/selected. That narrower scope for the special case seems more likely to be universally beneficial (though if "all collections always expanded" is really what you want, maybe we should consider a config setting for that so it doesn't change the default behavior for users that won't want it).
|
I see, yes the intention here was to only open the one and not many. I'll rethink. |
|
Thanks, @siiriylonen! Let me know if I can be of any assistance as you revise. Also note that if you start up the default test environment (with |
There was a problem hiding this comment.
Thanks, @siiriylonen -- this is looking much better. I just have one more question, which might or might not actually be relevant. :-)
| if ($hasChildren) { | ||
| $liClasses[] = 'hierarchy-tree__parent'; | ||
| } | ||
| if ('collection' === $node->type) { | ||
| $liClasses[] = 'hierarchy-tree__collection'; | ||
| $icon = 'hierarchy-collection'; | ||
| if ($nodeId === 0) { |
There was a problem hiding this comment.
Should we also check that $parentNodeId is empty here if we only want to impact the very top of the tree? It seems to be working fine for me as-is in my test environment, but that data is very simple... I'm not too familiar with the code (so @EreMaijala may have a better-informed opinion), but since there is recursion here, I thought there might be reason to add an extra restriction.
There was a problem hiding this comment.
Thanks, @siiriylonen. This is looking good to me now, but I have one small optimization suggestion below.
| if ($hasChildren) { | ||
| $liClasses[] = 'hierarchy-tree__parent'; | ||
| } | ||
| if ('collection' === $node->type) { | ||
| $liClasses[] = 'hierarchy-tree__collection'; | ||
| $icon = 'hierarchy-collection'; | ||
| if ($nodeId === 0 && !$parentNodeId && count($nodes) === 1) { |
There was a problem hiding this comment.
Would it be a good idea to set $nodeCount = count($nodes); above the foreach loop and then use the variable here, so if there are a large number of nodes to check, we only need to perform the count operation a single time?
demiankatz
changed the title
Open collection level by default in hierarchy tree for user convenience.